Conversation
- Add Lets-Plot Compose dependencies (v3.0.1) for JVM Desktop and Android - Create PlotDSL parser (ggplot2-inspired YAML/JSON format) - Add expect/actual LetsPlotBlockRenderer for cross-platform rendering - JVM/Android: Full Lets-Plot Compose rendering - iOS/JS/WASM: Fallback to code display - Create PlotDSLAgent sub-agent for LLM chart generation - Add PlotDSLCli for CLI testing - Add unit tests for PlotParser - Add integration tests for LLM PlotDSL generation Supported chart types: bar, line, scatter, histogram, boxplot, area, density Supported themes: default, minimal, classic, dark, light, void
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces PlotDSLAgent, a new agent for generating PlotDSL chart code from natural language descriptions. It adds conditional agent registration in CodingAgent, extends the SubAgent base class with an isAvailable property, implements platform-specific LetsPlot renderers with fallback support, and provides a CLI tool for standalone chart generation via LLM. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodingAgent
participant PlotDSLAgent
participant LLM as KoogLLMService
participant PlotParser
participant Renderer as Platform Renderer
User->>CodingAgent: Initialize with tool request
alt PlotDSLAgent.isAvailable == true
CodingAgent->>PlotDSLAgent: Register as sub-agent
Note over CodingAgent: Register in toolRegistry & subAgentManager
else Platform unsupported
CodingAgent-->>CodingAgent: Skip registration
end
User->>CodingAgent: Execute with PlotDSL request
CodingAgent->>PlotDSLAgent: Execute with PlotDSLContext
PlotDSLAgent->>PlotDSLAgent: validateInput(context)
PlotDSLAgent->>LLM: Generate PlotDSL code (with prompt)
loop Retry on validation failure (max 3 attempts)
LLM-->>PlotDSLAgent: Generated code
PlotDSLAgent->>PlotDSLAgent: validatePlotDSL()
alt Validation fails
PlotDSLAgent->>PlotDSLAgent: buildRetryPrompt(errors)
PlotDSLAgent->>LLM: Regenerate with feedback
else Validation passes
break
end
end
end
PlotDSLAgent-->>CodingAgent: Return formatted output
CodingAgent->>PlotParser: Parse code block into PlotConfig
PlotParser-->>Renderer: Provide PlotConfig
alt Platform supports LetsPlot (JVM/Android)
Renderer->>Renderer: buildLetsPlotFigure(config)
Renderer-->>User: Render interactive chart
else Platform unavailable
Renderer-->>User: Display code with fallback message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR integrates Lets-Plot Compose for statistical data visualization across the multiplatform codebase, adding support for generating and rendering ggplot2-inspired charts from natural language descriptions.
Key changes:
- Introduces PlotDSL, a YAML-based DSL for defining statistical plots that's optimized for LLM token efficiency
- Implements PlotDSLAgent SubAgent that generates chart code from natural language using LLM with retry/self-correction
- Provides full Lets-Plot Compose rendering on JVM Desktop and Android, with graceful fallback to code display on iOS/JS/WASM
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/PlotData.kt | Defines data models (PlotConfig, PlotGeom, PlotTheme, etc.) for PlotDSL serialization |
| mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/PlotParser.kt | Implements parser for YAML/JSON PlotDSL with fallback to simple format |
| mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.kt | Defines expect/actual interface for cross-platform chart rendering |
| mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.jvm.kt | JVM implementation using Lets-Plot Compose for full chart rendering |
| mpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.android.kt | Android implementation using Lets-Plot Compose (nearly identical to JVM) |
| mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.wasmJs.kt | WASM fallback showing PlotDSL code instead of rendering |
| mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.js.kt | JS fallback using HTML elements to display code |
| mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/LetsPlotBlockRenderer.ios.kt | iOS fallback showing PlotDSL code |
| mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.kt | SubAgent that generates PlotDSL from natural language with validation and retry logic |
| mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/core/SubAgent.kt | Adds isAvailable property for platform-specific SubAgent availability |
| mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/core/SubAgentManager.kt | Updates registration to check isAvailable before registering SubAgents |
| mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt | Integrates PlotDSLAgent with conditional registration based on platform |
| mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/PlotDSLCli.kt | CLI tool for testing PlotDSL generation from command line |
| mpp-ui/build.gradle.kts | Adds Lets-Plot Compose dependencies for JVM and Android platforms |
| mpp-ui/src/jvmTest/kotlin/cc/unitmesh/devins/ui/compose/sketch/letsplot/PlotParserTest.kt | Comprehensive unit tests for PlotParser covering all formats |
| mpp-ui/src/jvmTest/kotlin/cc/unitmesh/devins/ui/integration/PlotDSLGenerationIntegrationTest.kt | Integration tests for end-to-end LLM-based chart generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| subAgentManager.registerSubAgent(plotDSLAgent) |
There was a problem hiding this comment.
The conditional registration logic is inconsistent. PlotDSLAgent is registered to toolRegistry only when isAvailable is true (lines 139-142), but then unconditionally registered to subAgentManager (line 144). This could lead to the SubAgent being available in the manager but not as a tool. Either both should be conditional, or the subAgentManager.registerSubAgent() call should handle the availability check internally (which it does based on the SubAgentManager.kt changes).
| } | |
| subAgentManager.registerSubAgent(plotDSLAgent) | |
| subAgentManager.registerSubAgent(plotDSLAgent) | |
| } |
| implementation("org.jetbrains.lets-plot:lets-plot-kotlin-kernel:4.12.0") | ||
| implementation("org.jetbrains.lets-plot:lets-plot-common:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:canvas:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:plot-raster:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:lets-plot-compose:3.0.1") |
There was a problem hiding this comment.
Same version mismatch issue: lets-plot-kotlin-kernel is at version 4.12.0 while other Lets-Plot dependencies are at 4.8.1. This is the same issue as in jvmMain dependencies. Ensure consistent versions across all Lets-Plot dependencies for both JVM and Android platforms.
| @SerialName("scatter") | ||
| SCATTER, |
There was a problem hiding this comment.
[nitpick] The PlotGeom enum contains both POINT and SCATTER which are treated as identical in the parser (line 190 of PlotParser.kt maps both "point" and "scatter" to PlotGeom.POINT). Having both enum values is redundant and could cause confusion. Consider removing SCATTER and only keeping POINT, or document clearly why both are needed (e.g., for different serialization scenarios).
| @SerialName("scatter") | |
| SCATTER, |
| private fun validatePlotDSL(code: String): PlotDSLValidationResult { | ||
| val errors = mutableListOf<String>() | ||
| val warnings = mutableListOf<String>() | ||
|
|
||
| // Check if code is empty | ||
| if (code.isBlank()) { | ||
| errors.add("Generated code is empty") | ||
| return PlotDSLValidationResult(false, errors, warnings) | ||
| } | ||
|
|
||
| // Check for required sections | ||
| val hasDataSection = code.contains("data:") || code.contains("\"data\"") | ||
| val hasGeomSection = code.contains("geom:") || code.contains("\"geom\"") | ||
|
|
||
| if (!hasDataSection) { | ||
| errors.add("Missing 'data' section - chart needs data to visualize") | ||
| } | ||
|
|
||
| if (!hasGeomSection) { | ||
| warnings.add("Missing 'geom' section - using default chart type") | ||
| } | ||
|
|
||
| // Check for valid YAML/JSON structure | ||
| val hasValidStructure = (code.contains("plot:") || code.startsWith("{") || | ||
| code.contains("data:") || code.contains("title:")) | ||
|
|
||
| if (!hasValidStructure) { | ||
| errors.add("Invalid PlotDSL structure - should be YAML or JSON format") | ||
| } |
There was a problem hiding this comment.
The validation logic uses simple string matching (code.contains("data:")) which could produce false positives if these strings appear in comments, string literals, or field values. Consider using the PlotParser itself for validation (e.g., PlotParser.parse(code) != null) to ensure the code is actually parseable, rather than relying on string matching heuristics.
| private fun buildLetsPlotFigure(config: PlotConfig): Figure { | ||
| val data = config.data.toMap() | ||
| val aes = config.aes | ||
|
|
||
| // Start with base plot | ||
| var plot: Plot = letsPlot(data) | ||
|
|
||
| // Add geometry layer | ||
| plot = when (config.geom) { | ||
| PlotGeom.POINT, PlotGeom.SCATTER -> { | ||
| plot + geomPoint { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.color?.let { color = it } | ||
| aes?.size?.let { size = it } | ||
| aes?.shape?.let { shape = it } | ||
| aes?.alpha?.let { alpha = it } | ||
| } | ||
| } | ||
| PlotGeom.LINE -> { | ||
| plot + geomLine { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.color?.let { color = it } | ||
| aes?.group?.let { group = it } | ||
| } | ||
| } | ||
| PlotGeom.BAR -> { | ||
| plot + geomBar(stat = Stat.identity) { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.fill?.let { fill = it } | ||
| aes?.color?.let { color = it } | ||
| } | ||
| } | ||
| PlotGeom.HISTOGRAM -> { | ||
| plot + geomHistogram { | ||
| aes?.x?.let { x = it } | ||
| aes?.fill?.let { fill = it } | ||
| aes?.color?.let { color = it } | ||
| } | ||
| } | ||
| PlotGeom.BOXPLOT -> { | ||
| plot + geomBoxplot { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.fill?.let { fill = it } | ||
| aes?.color?.let { color = it } | ||
| } | ||
| } | ||
| PlotGeom.AREA -> { | ||
| plot + geomArea { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.fill?.let { fill = it } | ||
| aes?.group?.let { group = it } | ||
| } | ||
| } | ||
| PlotGeom.DENSITY -> { | ||
| plot + geomDensity { | ||
| aes?.x?.let { x = it } | ||
| aes?.fill?.let { fill = it } | ||
| aes?.color?.let { color = it } | ||
| } | ||
| } | ||
| PlotGeom.HEATMAP -> { | ||
| plot + geomTile { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.fill?.let { fill = it } | ||
| } | ||
| } | ||
| PlotGeom.PIE -> { | ||
| // Lets-Plot doesn't have native pie, use bar with coord_polar | ||
| plot + geomBar(stat = Stat.identity) { | ||
| aes?.x?.let { x = it } | ||
| aes?.y?.let { y = it } | ||
| aes?.fill?.let { fill = it } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add additional layers if present | ||
| config.layers?.forEach { layer -> | ||
| val layerData = layer.data?.toMap() ?: data | ||
| val layerAes = layer.aes ?: aes | ||
|
|
||
| plot = when (layer.geom) { | ||
| PlotGeom.POINT, PlotGeom.SCATTER -> { | ||
| plot + geomPoint(data = layerData) { | ||
| layerAes?.x?.let { x = it } | ||
| layerAes?.y?.let { y = it } | ||
| layerAes?.color?.let { color = it } | ||
| } | ||
| } | ||
| PlotGeom.LINE -> { | ||
| plot + geomLine(data = layerData) { | ||
| layerAes?.x?.let { x = it } | ||
| layerAes?.y?.let { y = it } | ||
| layerAes?.color?.let { color = it } | ||
| } | ||
| } | ||
| else -> plot | ||
| } | ||
| } | ||
|
|
||
| // Add title and labels | ||
| config.title?.let { plot = plot + ggtitle(it, config.subtitle) } | ||
| if (config.xLabel != null || config.yLabel != null) { | ||
| plot = plot + labs(x = config.xLabel, y = config.yLabel) | ||
| } | ||
|
|
||
| // Add theme | ||
| plot = plot + when (config.theme) { | ||
| PlotTheme.MINIMAL -> themeMinimal() | ||
| PlotTheme.CLASSIC -> themeClassic() | ||
| PlotTheme.DARK -> themeBW() // Closest to dark | ||
| PlotTheme.LIGHT -> themeLight() | ||
| PlotTheme.VOID -> themeVoid() | ||
| PlotTheme.DEFAULT -> themeGrey() | ||
| } | ||
|
|
||
| // Add size | ||
| val width = config.width ?: 400 | ||
| val height = config.height ?: 300 | ||
| plot = plot + ggsize(width, height) | ||
|
|
||
| // Add color scale if specified | ||
| config.colorScale?.let { scale -> | ||
| plot = when (scale.type) { | ||
| ColorScaleType.DISCRETE -> { | ||
| scale.colors?.let { colors -> | ||
| plot + scaleFillManual(values = colors) | ||
| } ?: plot | ||
| } | ||
| ColorScaleType.CONTINUOUS, ColorScaleType.GRADIENT -> { | ||
| if (scale.low != null && scale.high != null) { | ||
| plot + scaleFillGradient(low = scale.low, high = scale.high) | ||
| } else { | ||
| plot | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return plot | ||
| } |
There was a problem hiding this comment.
The buildLetsPlotFigure function is duplicated between JVM and Android implementations with only minor differences (default heights: 300 vs 250, default widths: 400 vs 350). Consider extracting this ~150 line function into a common module and parameterizing the platform-specific default values to reduce code duplication and maintenance burden.
| // Note: initializeWorkspace is already called in init block, no need to call again here | ||
| // The buildContext() will handle MCP tools initialization if needed |
There was a problem hiding this comment.
[nitpick] The comment on line 155 is misleading. It states "initializeWorkspace is already called in init block, no need to call again here," but this comment describes removed code behavior rather than explaining why the code was removed or what the current approach is. Consider removing this comment or updating it to explain the current initialization strategy more clearly.
| // Note: initializeWorkspace is already called in init block, no need to call again here | |
| // The buildContext() will handle MCP tools initialization if needed | |
| // Workspace initialization is performed asynchronously in the init block. | |
| // MCP tools initialization is handled within buildContext if needed. |
| implementation("org.jetbrains.lets-plot:lets-plot-common:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:canvas:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:plot-raster:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:lets-plot-image-export:4.8.1") | ||
| implementation("org.jetbrains.lets-plot:lets-plot-compose:3.0.1") |
There was a problem hiding this comment.
Version mismatch in Lets-Plot dependencies. The lets-plot-kotlin-kernel is at version 4.12.0 while lets-plot-common, canvas, and plot-raster are at version 4.8.1. Consider using consistent versions across all Lets-Plot dependencies to avoid potential compatibility issues. Check the official Lets-Plot Compose documentation for the correct version combinations.
| implementation("org.jetbrains.lets-plot:lets-plot-common:4.8.1") | |
| implementation("org.jetbrains.lets-plot:canvas:4.8.1") | |
| implementation("org.jetbrains.lets-plot:plot-raster:4.8.1") | |
| implementation("org.jetbrains.lets-plot:lets-plot-image-export:4.8.1") | |
| implementation("org.jetbrains.lets-plot:lets-plot-compose:3.0.1") | |
| implementation("org.jetbrains.lets-plot:lets-plot-common:4.12.0") | |
| implementation("org.jetbrains.lets-plot:canvas:4.12.0") | |
| implementation("org.jetbrains.lets-plot:plot-raster:4.12.0") | |
| implementation("org.jetbrains.lets-plot:lets-plot-image-export:4.12.0") | |
| implementation("org.jetbrains.lets-plot:lets-plot-compose:4.12.0") |
| } catch (e: Exception) { | ||
| parseSimpleFormat(trimmed) | ||
| } |
There was a problem hiding this comment.
The error handling in the parse function silently falls back to parseSimpleFormat for all exceptions (line 46-47). This could mask genuine errors like network issues, out of memory, or corrupted data. Consider logging the exception or at least distinguishing between parsing failures (which should fallback) and other types of exceptions (which should be propagated or logged).
|
|
||
| val yamlContent = configFile.readText() | ||
| val yaml = Yaml(configuration = com.charleskorn.kaml.YamlConfiguration(strictMode = false)) | ||
| val config = yaml.decodeFromString(AutoDevConfig.serializer(), yamlContent) |
There was a problem hiding this comment.
Missing import or definition of AutoDevConfig and LLMConfigEntry. These classes are used but not imported or defined in this file. They should either be imported from cc.unitmesh.server.cli.DocumentCli or extracted into a shared location to avoid duplication.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.